Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overlap and Add algorithm for fast convolution of long array #695

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

eleroy
Copy link

@eleroy eleroy commented Nov 25, 2024

Hi everyone,

This an implementation of the oaconvolve method that may be faster than convolve espiecielly when dealing with long arrays.

Few things I'm not sure:

  • I don't think this will work without complex support and I'm not sure how to change the code to do so
  • I'm not sure there si a need for m_free at the end of the method
  • I just realized there are micropython contributing guidelines that I probably did not completely follow

The method takes the same arguments as the convolve method.

@v923z
Copy link
Owner

v923z commented Nov 25, 2024

I'm a bit confused: we agreed that oaconvolve is part of scipy.signal, so numpy.c, and filter.c shouldn't be modified at all. You should also add tests, documentation, and move the version number in ulab.c.

@eleroy
Copy link
Author

eleroy commented Nov 25, 2024

Pff...sorry i forgot to move the method to signal, left it where i built it in filter.

I also saw it didn't built since i added the m_free that was not well used (IS it useful to free the buffer at the end or it will be free anyway when the method returns?)

I'll modify the PR to address your remarks.

@v923z
Copy link
Owner

v923z commented Nov 25, 2024

And I think that the problem of complexes has to be solved. It wouldn't make sense to include complex support for the sake of a single method.

@v923z
Copy link
Owner

v923z commented Nov 25, 2024

@v923z
Copy link
Owner

v923z commented Nov 25, 2024

I also saw it didn't built since i added the m_free that was not well used (IS it useful to free the buffer at the end or it will be free anyway when the method returns?)

In this case, it shouldn't really matter.

…t complex. It should also work if there is no complex support. Revert filter+numpy changes
@eleroy
Copy link
Author

eleroy commented Nov 26, 2024

added:

  • Support without complex number
  • Documentation for oaconvolve
  • Bump ulab version
  • Test for oaconvolve (same as convolve but with reduced absolute tolerance, it seems the method is not as accurate as convolve)

@@ -2,9 +2,10 @@
scipy.signal
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation is actually generated from this: https://github.com/v923z/micropython-ulab/blob/master/docs/scipy-signal.ipynb. The jupyter notebook allows you to run micropython code directly, so you don't have to copy anything.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha ok, I made the changes in the jupyter notebook but I'm not sure how to run it (my system is windows).

code/scipy/signal/signal.c Outdated Show resolved Hide resolved
code/scipy/signal/signal.c Outdated Show resolved Hide resolved
@v923z
Copy link
Owner

v923z commented Nov 26, 2024

added:

* Support without complex number

* Documentation for oaconvolve

* Bump ulab version

* Test for oaconvolve (same as convolve but with reduced absolute tolerance, it seems the method is not as accurate as convolve)

Many thanks! I think it's almost ready to go. I added a couple of small comments. If those can be resolved, this could be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants